Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test output as comment #335

Merged
merged 11 commits into from
Jul 26, 2024
Merged

Test output as comment #335

merged 11 commits into from
Jul 26, 2024

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Jul 23, 2024 •
edited
Loading

Based on #332 this is my proposal with some tests as example and works with multiline output.

In this way:

  • If there is a comment in the code that starts with // Output it will get that content for the test check
  • Otherwise
    • If there is a *.output.txt file will read from that
      • Otherwise will use Succeded as string for the check

I think that in this case we can simplify a lot and cover all the needs for the future.

If I get an approval I will move on to migrate (again) all the tests (also for validity) with the contribute guide.

Ph0enixKM
Ph0enixKM requested changes Jul 23, 2024
View reviewed changes
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small changes. I'll think more about the actual implementation. I thought that we count only the comments at the top to be the output and not the comments that start with "Output". Give me some time to let that sink in

src/tests/stdlib.rs Outdated Show resolved Hide resolved
src/tests/stdlib.rs Show resolved Hide resolved
@Mte90 @Ph0enixKM
Update src/tests/stdlib.rs
e81b481 
Co-authored-by: Phoenix Himself <[email protected]>
@Mte90
Copy link
Member Author

Mte90 commented Jul 23, 2024 •
edited
Loading

About Output on top, it is just to give a bit of context in case someone else open those files and doesn't understand this comments.
Also in case it is needed to add some documentation as some tests has already.

b1ek
b1ek requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gotta be explained in CONTRIBUTING.md tho

@Mte90
Copy link
Member Author

Mte90 commented Jul 26, 2024

this gotta be explained in CONTRIBUTING.md tho

Yes I will do that when we define how the comment is structured.

@Mte90 Mte90 requested review from b1ek and Ph0enixKM July 26, 2024 12:13
Ph0enixKM
Ph0enixKM approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

src/tests/stdlib.rs Outdated Show resolved Hide resolved
b1ek
b1ek requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just like i said, it gotta be explained in CONTRIBUTING.md

@Mte90 Mte90 requested a review from b1ek July 26, 2024 13:34
@b1ek b1ek merged commit 038a5b3 into amber-lang:master Jul 26, 2024
1 check passed
@Mte90 Mte90 deleted the test-comment-output branch September 5, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b1ek b1ek b1ek approved these changes

@Ph0enixKM Ph0enixKM Ph0enixKM approved these changes

@KrosFire KrosFire Awaiting requested review from KrosFire

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mte90 @Ph0enixKM @b1ek